Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol lua API #3461

Closed
wants to merge 25 commits into from
Closed

Conversation

EvilHero90
Copy link
Contributor

@EvilHero90 EvilHero90 commented May 15, 2021

This implements a new Protocol service, which gives us a bridge to interact with the server directly with an outside API of your preference with lua code.

What does that mean do you think?
Basicly we can now send lua code directly from an outside API into the server, a good example would be that we have an administration page on our AAC's where we insert lua code and then send it through the API to the server and it immediatly gets executed, this would make shops etc. a lot more efficient and easier to handle, or you can even administrate your server from your website without the need to touch any kind of files on your server.

Why did I chose protocol over http server?
Well... I thought why should we implement an entire http server when our protocol can easily handle all this already?

What are your final plans with this?
This actually opens us doors to quite a few new options, if we ever fully convert everything to revscriptsys then we are able to deploy servers easily without even the need to have any lua file on your hdd, we can easily fetch them directly from any website we want such as github (github raw's to be precise) or otland (if we add support for it)
I know this kinda sounds complex but it would also make it a lot easier for people sharing their scripts to keep them updated for all people who use them as you'll always have the newest version then.

Do you provide a sample API?
@Znote wrote a basic php API for testing purposes, which we will extend on
https://github.com/EvilHero90/TFS-lua-API/tree/main

This is how the current structure looks like: (might still change while WIP)

/*
* List of Packets used for communication
* --------------------------------------------------------------------
* 
* Server ----> API (structure of packet)
* --------------------------------------------------------------------
* 100 => ping
* 101 => sending a callback message (string)
* 102 => sending lua error back to API (string)
* 103 => request to exchange lua code, if the API has any
* --------------------------------------------------------------------
* 
* API ----> Server (structure of packet)
* --------------------------------------------------------------------
* 100 => pong
* 101 => sending raw string lua code with immediate execution (string[name], string[data])
* 102 => save server
* 103 => clean server
* 104 => close server
* 105 => start raid (string)
* 200 => reload scripts
* --------------------------------------------------------------------
*/

Here is a screenshot from a testing API @Znote made for me (thanks btw)
luaapi1

This also supports sending error messages back to the API if there are any
errorresponse

I'm completly open for suggestions and such, for now this is WIP and probably a few things will change here and there but it's ready to be tested.

- added ping & pong, that we can be sure API is connected
- added a new event "onLuaApiResponse" so we can handle recvbytes easily there
- cleaned up the code and removed some hardcoded stuff
- Now the server expects at packet 101 name of the file first as string and then the data
- enhanced the error handling, that way we can easily see which file throws the error
- added more packet handling
added this accidently back when I started working towards this PR
@EvilHero90 EvilHero90 added feature New feature or functionality needs-testing Needs testing with screenshots decisions Debatable/disputable labels May 15, 2021
data/globalevents/scripts/serversave.lua Show resolved Hide resolved
src/events.cpp Outdated Show resolved Hide resolved
src/events.h Outdated Show resolved Hide resolved
src/luascript.cpp Show resolved Hide resolved
g_events->eventMiscOnLuaApiResponse(recvbyte, name, data);

switch (recvbyte) {
case 100: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hex values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make it hex once the testing stage is through, not sure if we change the values at some point, or what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client packets are parsed using hex values too, so for consistency's sake it should stay that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned, I'll change them to hex once they're 100% set in stone and wont get changed

src/protocolluaapi.cpp Show resolved Hide resolved
converted everything related to `misc` with `game`
@EvilHero90
Copy link
Contributor Author

EvilHero90 commented May 15, 2021

Here is the basic php test page @Znote made, if anyone wants to give it a shot.
https://github.com/EvilHero90/TFS-lua-API/tree/main

@MillhioreBT
Copy link
Contributor

And what about security regarding this API?
I say it has always been known that web pages that are not well protected can be easily hacked and therefore in that case someone could run Lua code in the engine and do what they want

It is just something that crossed my mind, but surely we will not need a sandbox to execute the code sent from this API

@EvilHero90
Copy link
Contributor Author

This is as secure as it can get, we already limit the connection to localhost or bind it to a specific ip, if someone by accident opens the port, it would still refuse the connection due to ip restriction.
Security wise it shouldn't be our problem to begin with, if someone gets hold of your website, then you are completly doomed anyway.

- reverted changes regarding reload, since it doesn't make sense to reload other interfaces from outside anyway
- moved the event into default handling, to be able to print out unknown packets
config.lua.dist Outdated Show resolved Hide resolved
@DSpeichert
Copy link
Member

Considering how even cipbia moved to standardized protocols like ProtoBuffers, I am very unconvinced it's a good idea to design a new protocol for this, even if it's so simple and pure as this one.

@Znote
Copy link
Member

Znote commented May 29, 2021

Considering how even cipbia moved to standardized protocols like ProtoBuffers, I am very unconvinced it's a good idea to design a new protocol for this, even if it's so simple and pure as this one.

Can you refer to a PR that uses protobuffers?
I find this API to be small, fresh, flexible and consistent with our current protocols, so what is not to love?

@MillhioreBT
Copy link
Contributor

I have been investigating how to work with ProtoBuffers and it seems to be a pretty good technology, although it is robust, it seems very redundant, I think I should investigate more and do real tests with TFS, to be able to comment on the potential that it can bring to TFS, but for now this new protocol looks very promising and simple

@DSpeichert
Copy link
Member

Can you refer to a PR that uses protobuffers?
I find this API to be small, fresh, flexible and consistent with our current protocols, so what is not to love?

What do you mean? There's been talks about that in other PRs and an attempt to use protobuffers for other serialization. Technically, for APIs there's a standard based around protobuffers called gRPC.

What's not to love? I personally don't like any part of this. While it's lighter than using a full-fledged existing solution, designing a custom raw/binary protocol requires much more than this to make it well defined. It's simply not necessary. I'm already confused by the documentation: what is "API", what is "server"?

How would you handle authentication? Encryption, when desired? HTTP would allow wrapping in a reverse proxy with mutual TLS auth or other known solutions, a binary TCP protocol isn't easy to wrap to add auth/encryption, which eventually someone is going to want (there's already tons of threads on the forums where the website and server run on different hosts in different places).

Why only consider "website"? There used to be a TFS Admin protocol and we can do better this time. Same with the status protocol, which originally had a few binary "commands" (e.g. list players) but then switched to XML. gRPC/ProtoBuffers didn't exist back then.

I'm not saying ProtoBuffers/gRPC is the only way, it's just an example that I think would work if there is a good way to integrate it with Lua in TFS. It is known to work and has support in many languages. HTTP wouldn't be bad either. A custom protocol these days is simply rarely a good choice because designing, documenting and implementing one from scratch is no small feat.

@EvilHero90
Copy link
Contributor Author

Can you refer to a PR that uses protobuffers?
I find this API to be small, fresh, flexible and consistent with our current protocols, so what is not to love?

What do you mean? There's been talks about that in other PRs and an attempt to use protobuffers for other serialization. Technically, for APIs there's a standard based around protobuffers called gRPC.

What's not to love? I personally don't like any part of this. While it's lighter than using a full-fledged existing solution, designing a custom raw/binary protocol requires much more than this to make it well defined. It's simply not necessary. I'm already confused by the documentation: what is "API", what is "server"?

Ok so you rather want to add an entire http server or gRPC handling, which we both don't use anyway and bloat tfs more up than it already is? we rely on protocol anyway so we should make use of it, instead of waiting another 10 years for someone to re write everything to gRPC.

How would you handle authentication? Encryption, when desired? HTTP would allow wrapping in a reverse proxy with mutual TLS auth or other known solutions, a binary TCP protocol isn't easy to wrap to add auth/encryption, which eventually someone is going to want (there's already tons of threads on the forums where the website and server run on different hosts in different places).

This is already limited to either localhost or to bind a specific IP to be able to communicate, so there is no need for authentication.
Everything you mentioned here is part of the API anyway, if the API is poorly protected that's none of our concern.

Why only consider "website"? There used to be a TFS Admin protocol and we can do better this time. Same with the status protocol, which originally had a few binary "commands" (e.g. list players) but then switched to XML. gRPC/ProtoBuffers didn't exist back then.

I've never stated anywhere that I only consider "websites" we just decided to quickly show of a simple API which can be easily included into any AAC, if you rather want to write an API in c# or any other language then everyone can feel free to do that.

I'm not saying ProtoBuffers/gRPC is the only way, it's just an example that I think would work if there is a good way to integrate it with Lua in TFS. It is known to work and has support in many languages. HTTP wouldn't be bad either. A custom protocol these days is simply rarely a good choice because designing, documenting and implementing one from scratch is no small feat.

"designing, documenting and implementing" all of what you mentioned there is even more work for what you suggest, we seem to be forgetting that "tfs" is a game server and not an allround package which should include several things in one program.

@UReddington
Copy link

I'm using this with a discord bot and it works like a charm, just an observation, it seems like the ip verification is broken, i've changed the ip on the config.lua to an external host and it wasn't working at all (i had to just remove the localhost check, with this it works as intended)

src/protocolluaapi.cpp Outdated Show resolved Hide resolved
@DSpeichert
Copy link
Member

Quick and simple is exactly what it is. It works, I'm sure (though I haven't tried myself).

Personally, I would prefer to add entire HTTP stack and it's been proposed in #2010. While the amount of initial "bloat" is larger indeed, I believe the flexibility and power of a standard HTTP interface will be worth it.

What I see here is a potential for blocking limitations (like the aforementioned authentication/encryption issues) that aren't easy to change later. Adding byte-encoded API functions (?) is very crude. This protocol that we keep calling "protocol" here is literally pushing bytes back and forth, leaving the rest to a developer. The developer is usually more of an end-user who would prefer to use something more common that is well known or well documented (such as HTTP) instead of figuring out a raw byte-based custom "protocol". Eventually, people will want authentication, or JSON encoding, or multiplexing, compression, streaming, etc. I think it's easier to adapt based on a standard.

Long story short, all that bloat that you don't have here might eventually be necessary down there later.
We may be waiting for 10 years for something good and that's exactly why we should carefully think before adding anything. There is no time to go back and redo.

@MillhioreBT
Copy link
Contributor

I'm using this with a discord bot and it works like a charm, just an observation, it seems like the ip verification is broken, i've changed the ip on the config.lua to an external host and it wasn't working at all (i had to just remove the localhost check, with this it works as intended)

Very Nice! working perfectly

GIF test discord BOT

GIF 03-06-2021 04-58-07 p  m

@Znote
Copy link
Member

Znote commented Jun 5, 2021

Personally, I would prefer to add entire HTTP stack and it's been proposed in #2010. While the amount of initial "bloat" is larger indeed, I believe the flexibility and power of a standard HTTP interface will be worth it.

I have to agree, I am hyped for an HTTP stack API like this one. Perhaps @EvilHero90 could attempt to continue @djarek's work?

Quote @djarek

There are significant differences between the old Beast API and the new one, so this needs a rewrite.

This along with radio silence for 2 years made the HTTP API a pipe-dream. I would prefer http api over this one, but I'll take anything over nothing.

@MillhioreBT
Copy link
Contributor

Personally, I would prefer to add entire HTTP stack and it's been proposed in #2010. While the amount of initial "bloat" is larger indeed, I believe the flexibility and power of a standard HTTP interface will be worth it.

I have to agree, I am hyped for an HTTP stack API like this one. Perhaps @EvilHero90 could attempt to continue @djarek's work?

Quote @djarek

There are significant differences between the old Beast API and the new one, so this needs a rewrite.

This along with radio silence for 2 years made the HTTP API a pipe-dream. I would prefer http api over this one, but I'll take anything over nothing.

If @djarek changes are more secure, I will support the idea, I would like to know more about it to help with this!

@UReddington
Copy link

Following some recent feedback, i would suggest adding a config variable to prevent the direct code execution (config variable, just a thing that would kill the connection if they doesn't want that), as we have already a trust problem of what he owners can execute if they use third party tools

@Codinablack
Copy link
Contributor

Following some recent feedback, i would suggest adding a config variable to prevent the direct code execution (config variable, just a thing that would kill the connection if they doesn't want that), as we have already a trust problem of what he owners can execute if they use third party tools

I agree with this, the server owner should have the right to choose if they want this enabled or not, if it gets added.

We are now able to parse http requests pretty easy from api to server

I've also moved everything from Events to GlobalEvents, it feels a lot better placed there.

I've added a file called api.lua inside scripts/http it provides a good example on how to use the GlobalEvent and how to parse data from it
- We now have the option to disable the api in config.lua
I also re organised the lua api part in it.
- added a bit better documentation on how packets are send & handled
- fixed ip matching, now you can have a custom ip set and stil be able to connect from localhost aswell
- added app handling for later development
- http requests are now fixed on packet 102, that way I could handle non existant packets callback messages
@Zbizu
Copy link
Contributor

Zbizu commented Jun 14, 2021

imo this would be great if the packet was treated as text (like opcodes right now) so the end user would be able to choose whether he wants to loadstring or just use the api in a different way

it would also be far safer than immediate execution and would also open some new possibilities like live information about ongoing events or new ways to interact with otclient/ot lists

@EvilHero90
Copy link
Contributor Author

EvilHero90 commented Jun 14, 2021

imo this would be great if the packet was treated as text (like opcodes right now) so the end user would be able to choose whether he wants to loadstring or just use the api in a different way

it would also be far safer than immediate execution and would also open some new possibilities like live information about ongoing events or new ways to interact with otclient/ot lists

this is already implemented now, we can basicly parse http requests, this way you can even add 3rd party software which connects through your api and give it a certain access to do stuff

the entrance post is outdated, I'll change it once I have a bit more time, you want to take a look at protocolluaapi.cpp/h, you'll find examples etc. there

Copy link
Member

@nekiro nekiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of all the "auto" there in the code. I prefer to have visible types when I look at variables, but that's my opinion.

/* We are searching if we can even find a registered GlobalEvent with the name
* This way we could push app names onward from the API to only execute the GlobalEvent which belongs to this app
*/
auto http = g_globalEvents->getEventMap(GLOBALEVENT_HTTPREQUEST);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt this an unnecessary copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea thanks, fixed in latest commit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok nvm, I just noticed I've already tried that and build failed (windows doesn't throw any error tho)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto& ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope throws an error aswell, I'll take a closer look soon

src/protocolluaapi.cpp Outdated Show resolved Hide resolved
src/protocolluaapi.cpp Outdated Show resolved Hide resolved
@EvilHero90
Copy link
Contributor Author

We wanted to modernize code and auto was one of the things which we talked about that we should use as much as we can, only if it makes the code completly unreadable but I agree in lots of cases I would prefer a strict type aswell

src/protocolluaapi.cpp Outdated Show resolved Hide resolved
@EvilHero90 EvilHero90 closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decisions Debatable/disputable feature New feature or functionality needs-testing Needs testing with screenshots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants